Soo?

Give this man a cookie!

Wooo, so does this PR give us full source maps with relative path assets, without the need for an absolute publicPath?

👏

@oller sure. Tested by using in real project, works like a charm.

Worked for me at first glance. Any reason why this isn't merged?
@Strate I suggest squashing commits into one

Thanks for the patch. Works like charm.

@sokra this is really helpful patch, as you can see. could you merge it? Or maybe I could somehow improve this patch?

bump. Need this patch

Come on, please merge :)

Pleeeeaaase! 🎁

It's been 5 months... Is there any reason for this not to be merged @sokra ?

yes. This is not a good solution. It should be solved on CSS AST. It's better to solve it before the style loader in the css loader

@sokra I can't find any code in css-loader that is client side, which is where this logic would need to reside so it knows the browser location at run time. Can you point us in the right direction?

@Strate great idea and great work!

I just submitted a very similar pull request which is opt-in via a style?fixUrls flag and has some unit tests to verify the behavior since I'm not parsing the CSS into an AST.

Anyone feel free to fork bendytree/style-loader and then use a git url in your package.json:

...
"style-loader": "git+https://git@github.com/YOUR_USERNAME/style-loader.git",
...

@sokra any guidance you could provide @Strate / @bendytree et al to solve this properly in the CSS AST as you suggested?

As for me, use css ast on client side would be an overkill. This patch used in real world project since it's creation without any problem. With keeping in mind that it used only in dev environment, I could say that it has sufficient quality to merge.

This is a huge issue on my project as well, can we get update why this hasn't been merged?

Adding an additional "please". Or at least provide feedback how this could be made better.

I'll work on this this afternoon. I have some ideas how to solve this correctly on AST level. But I'll have to test the performance impact.

Attention please.

I've found a solution for the problem without patching this loader.
Just add following line on the top of your topmost entry does the job:

__webpack_public_path__ = window.location.protocol + "//" + window.location.host + "/"

This line modifies output.publicPath option at browser runtime, adding current protocol and host. If you have a custom publicPath, just add it to the end of prefix.
This solution requires you to have no output.publicPath specified in config or command line.
If you need to pass publicPath from command line or webpack configuration file you can use environment variables together with DefinePlugin:

// webpack.config.js

module.exports = {
  pluginns: [
    new webpack.DefinePlugin({
      'process.env.WEBPACK_PUBLIC_PATH': JSON.stringify(process.env.WEBPACK_PUBLIC_PATH)
    })
  ]
}

// topmostEntryPoint.js
__webpack_public_path__ = window.location.protocol + "//" + window.location.host + "/" + process.env.WEBPACK_PUBLIC_PATH;

and run build with command:

WEBPACK_PUBLIC_PATH="your/public/path/" webpack

Also - There were compliance issues we needed to clear up before the CLA bot could be enabled, that was cleared up about 10 days ago.

To get the CLA signed, you just need to close this PR & open it again. That will trigger the CLA workflow again which should allow you to sign it.

Please note, before you close & open it again ensure the email in your local git config matches what you have configured in github.

I looks like this PR might be superseded by #124?

Yes. My take on all of this would be to land #124, put #165 in the next major & close #96 but that plan needs to be confirmed by the @webpack-contrib/org-maintainers

@d3viant0ne Yeah, sounds fine.

@d3viant0ne have you read this comment: #96 (comment)? This is how I solve this issue current in my project. I think it is enougth to move this trick to README.

It's workaround vs. fix where the workaround is only effective in some cases. This is an issue that needs to be resolved in code

#186 (#124) landed, is this the PR still relevant then? If @Strate please close and reopen to trigger the CLA Bot again and rebase against current master, otherwise if you lost interest in this PR in general / #186 (#124) solved the issue please confirm we can close

This should be closed in favor of #124 which has already been merged.